Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

deps: V8: cherry-pick 687d865fe251 #31007

Closed
wants to merge 1 commit into from
Closed

deps: V8: cherry-pick 687d865fe251 #31007

wants to merge 1 commit into from

Conversation

ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Dec 17, 2019 •
edited
Loading

Opening this to test performance and memory benefits. Upd: good so far, marking as ready.

This should reduce the memory load and improve the speed in cases that heavily rely on Buffer usage.

Refs: v8/v8@687d865
Refs: #1671

Original commit message:

[heap] Perform GCs on v8::BackingStore allocation

This adds heuristics to perform young and full GCs on allocation
of external ArrayBuffer backing stores.

Young GCs are performed proactively based on the external backing
store bytes for the young generation. Full GCs are performed only
if the allocation fails. Subsequent CLs will add heuristics to
start incremental full GCs based on the external backing store bytes.

This will allow us to remove AdjustAmountOfExternalMemory for
ArrayBuffers.

Bug: v8:9701, chromium:1008938
Change-Id: I0e8688f582989518926c38260b5cf14e2ca93f84
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1803614
Commit-Queue: Ulan Degenbaev <[email protected]>
Reviewed-by: Dominik Inführ <[email protected]>
Reviewed-by: Hannes Payer <[email protected]>
Cr-Commit-Position: refs/heads/master@{#65480}

@ChALkeR ChALkeR added v8 engine Issues and PRs related to the V8 dependency. memory Issues and PRs related to the memory management or memory footprint. performance Issues and PRs related to the performance of Node.js. labels Dec 17, 2019
@ChALkeR
Copy link
Member Author

ChALkeR commented Dec 17, 2019

/cc @nodejs/benchmarking

@ChALkeR ChALkeR mentioned this pull request Dec 17, 2019
Closed
@ChALkeR
Copy link
Member Author

ChALkeR commented Dec 17, 2019 •
edited
Loading

Benchmarks:

@ChALkeR
Copy link
Member Author

ChALkeR commented Dec 17, 2019 •
edited
Loading

Benchmarks (updated, complete):

buffers (complete):

compare-instance-method n=1000000 args=5 size=16                                *   7.00 %   ±6.81%  ±9.14% ±12.08%
compare-offset n=1000000 size=16386 method='slice'                              *  -6.24 %   ±5.99%  ±8.05% ±10.63%
concat n=800000 withTotalLength=0 pieceSize=256 pieces=16                     ***  24.22 %  ±11.06% ±14.71% ±19.15%
concat n=800000 withTotalLength=1 pieceSize=1 pieces=4                          *   1.96 %   ±1.65%  ±2.19%  ±2.86%
concat n=800000 withTotalLength=1 pieceSize=256 pieces=16                      **  19.40 %  ±11.76% ±15.65% ±20.37%
copy n=6000000 partial='false' bytes=8                                          *  -2.07 %   ±1.78%  ±2.37%  ±3.08%
copy n=6000000 partial='true' bytes=128                                         *   2.65 %   ±2.34%  ±3.12%  ±4.07%
creation n=600000 len=4096 type='slow-allocUnsafe'                              *  11.27 %   ±8.64% ±11.51% ±15.00%
creation n=600000 len=8192 type='fast-alloc'                                  ***  23.01 %   ±8.84% ±11.85% ±15.62%
creation n=600000 len=8192 type='fast-alloc-fill'                             ***  21.18 %   ±7.48% ±10.02% ±13.19%
creation n=600000 len=8192 type='fast-allocUnsafe'                            ***  10.57 %   ±4.46%  ±5.95%  ±7.77%
creation n=600000 len=8192 type='slow-allocUnsafe'                            ***  14.68 %   ±5.57%  ±7.44%  ±9.75%
fill n=20000 size=8192 type='fill(0)'                                           *   6.23 %   ±5.83%  ±7.76% ±10.11%
fill n=20000 size=8192 type='fill(Buffer.alloc(1), 0)'                          *   7.37 %   ±6.00%  ±8.02% ±10.49%
fill n=20000 size=8192 type='fill("t", "utf8")'                                 *  -6.35 %   ±5.94%  ±7.91% ±10.29%
indexof n=50000 type='string' encoding='ucs2' search='Alice'                    *  -7.07 %   ±7.02%  ±9.35% ±12.19%
indexof n=50000 type='string' encoding='ucs2' search='</i> to the Caterpillar'  *  -2.96 %   ±2.35%  ±3.12%  ±4.06%
indexof n=50000 type='string' encoding='ucs2' search='Ou est ma chatte?'        *  -8.47 %   ±6.75%  ±9.00% ±11.76%
normalize-encoding n=1000000 encoding='UTF16LE'                                 *   6.06 %   ±5.45%  ±7.27%  ±9.48%
read-with-byteLength byteLength=1 n=1000000 type='IntLE' buffer='fast'          *  -9.24 %   ±9.09% ±12.19% ±16.04%
swap n=1000000 len=2056 method='swap32' aligned='false'                       ***  10.16 %   ±4.74%  ±6.32%  ±8.24%
swap n=1000000 len=768 method='swap16' aligned='false'                          *  -2.86 %   ±2.86%  ±3.80%  ±4.95%
dataview-set n=1000000 type='Uint16BE'                                          *  -6.84 %   ±5.55%  ±7.41%  ±9.71%

18.85 false positives, when considering a  5% risk acceptance (*, **, ***),
3.77 false positives, when considering a   1% risk acceptance (**, ***),
0.38 false positives, when considering a 0.1% risk acceptance (***)

Most of * are likely flukes (there was a large number of benchmarks in the whole buffer set), see the expected false positives count.

net (complete):

net-c2s-cork dur=5 type='buf' len=1024                                                *  -16.61 %  ±14.59% ±19.41% ±25.27%
net-c2s dur=5 type='utf' len=64                                                       *    4.34 %   ±3.86%  ±5.14%  ±6.70%
net-pipe dur=5 type='asc' len=64                                                    ***   10.26 %   ±5.37%  ±7.16%  ±9.35%
net-s2c dur=5 recvbufgenfn='false' recvbuflen=0 type='buf' sendchunklen=256           *    5.83 %   ±5.10%  ±6.81%  ±8.90%
net-s2c dur=5 recvbufgenfn='false' recvbuflen=1048576 type='buf' sendchunklen=256     *   -2.24 %   ±2.11%  ±2.80%  ±3.65%
net-s2c dur=5 recvbufgenfn='false' recvbuflen=1048576 type='utf' sendchunklen=131072  *    1.97 %   ±1.81%  ±2.41%  ±3.14%
net-s2c dur=5 recvbufgenfn='true' recvbuflen=65536 type='buf' sendchunklen=131072     *    2.23 %   ±1.70%  ±2.26%  ±2.94%
tcp-raw-s2c dur=5 type='asc' len=16777216                                             *   -4.18 %   ±4.18%  ±5.56%  ±7.24%
tcp-raw-s2c dur=5 type='utf' len=16777216                                             *   -3.49 %   ±3.32%  ±4.42%  ±5.76%

6.25 false positives, when considering a   5% risk acceptance (*, **, ***),
1.25 false positives, when considering a   1% risk acceptance (**, ***),
0.12 false positives, when considering a 0.1% risk acceptance (***)

Again, most of * are likely flukes.

streams/pipe

Nothing.

fs/stream-throughput

read-stream-throughput size=1024 filesize=1048576000 encodingType='asc'      *   6.03 %       ±5.46%  ±7.27%   ±9.47%
read-stream-throughput size=1024 filesize=1048576000 encodingType='utf'     **   6.60 %       ±4.70%  ±6.28%   ±8.22%
read-stream-throughput size=1048576 filesize=1048576000 encodingType='asc'   *  10.39 %       ±7.98% ±10.63%  ±13.87%
read-stream-throughput size=1048576 filesize=1048576000 encodingType='buf'   *  12.97 %      ±11.28% ±15.02%  ±19.56%
read-stream-throughput size=65535 filesize=1048576000 encodingType='buf'    **  13.90 %      ±10.00% ±13.31%  ±17.33%

1.20 false positives, when considering a   5% risk acceptance (*, **, ***),
0.24 false positives, when considering a   1% risk acceptance (**, ***),
0.02 false positives, when considering a 0.1% risk acceptance (***)

@ChALkeR ChALkeR added the buffer Issues and PRs related to the buffer subsystem. label Dec 17, 2019
@ChALkeR ChALkeR marked this pull request as ready for review December 18, 2019 10:20
@ChALkeR ChALkeR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v10.x labels Dec 19, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 20, 2019 •
edited by BridgeAR
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/27821/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2718/

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27924/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 25, 2019 •
edited by BridgeAR
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/27934/ ✔️

BridgeAR pushed a commit that referenced this pull request Dec 25, 2019
@ChALkeR @BridgeAR
deps: V8: cherry-pick 687d865fe251
0e21c1e 
Original commit message:

    [heap] Perform GCs on v8::BackingStore allocation

    This adds heuristics to perform young and full GCs on allocation
    of external ArrayBuffer backing stores.

    Young GCs are performed proactively based on the external backing
    store bytes for the young generation. Full GCs are performed only
    if the allocation fails. Subsequent CLs will add heuristics to
    start incremental full GCs based on the external backing store bytes.

    This will allow us to remove AdjustAmountOfExternalMemory for
    ArrayBuffers.

    Bug: v8:9701, chromium:1008938
    Change-Id: I0e8688f582989518926c38260b5cf14e2ca93f84
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1803614
    Commit-Queue: Ulan Degenbaev <[email protected]>
    Reviewed-by: Dominik Inführ <[email protected]>
    Reviewed-by: Hannes Payer <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#65480}

PR-URL: #31007
Refs: v8/v8@687d865
Refs: #1671
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@BridgeAR

This comment has been minimized.

Sign in to view
@BridgeAR BridgeAR closed this Dec 25, 2019
@BridgeAR
Copy link
Member

I forced pushed it out again after realizing that the embedder string has not been updated.

@BridgeAR BridgeAR reopened this Dec 25, 2019
@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 25, 2019
@ChALkeR
Copy link
Member Author

ChALkeR commented Dec 26, 2019

@BridgeAR Oh, my bad re: v8_embedder_string.

The problem is that it did not work -- it's present in master: https://github.com/nodejs/node/commits/master

Will file two alternative PRs to fix that now.

ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Dec 26, 2019
@ChALkeR
deps: V8: bump v8_embedder_string for 0e21c1e
4cf5402 
0e21c1e has landed without a proper
v8_embedder_string bump, this is a follow-up fix.

Refs: 0e21c1e
Refs: nodejs#31007
@ChALkeR ChALkeR mentioned this pull request Dec 26, 2019
Closed
@ChALkeR
deps: V8: cherry-pick 687d865fe251
b7fa732 
Original commit message:

    [heap] Perform GCs on v8::BackingStore allocation

    This adds heuristics to perform young and full GCs on allocation
    of external ArrayBuffer backing stores.

    Young GCs are performed proactively based on the external backing
    store bytes for the young generation. Full GCs are performed only
    if the allocation fails. Subsequent CLs will add heuristics to
    start incremental full GCs based on the external backing store bytes.

    This will allow us to remove AdjustAmountOfExternalMemory for
    ArrayBuffers.

    Bug: v8:9701, chromium:1008938
    Change-Id: I0e8688f582989518926c38260b5cf14e2ca93f84
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1803614
    Commit-Queue: Ulan Degenbaev <[email protected]>
    Reviewed-by: Dominik Inführ <[email protected]>
    Reviewed-by: Hannes Payer <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#65480}

Refs: v8/v8@687d865
Refs: nodejs#1671
@ChALkeR ChALkeR force-pushed the chalker/v8-cherrypick-scavenges branch from f707f1e to b7fa732 Compare December 26, 2019 02:35
@ChALkeR
Copy link
Member Author

ChALkeR commented Dec 26, 2019 •
edited
Loading

This PR is now updated to include v8_embedder_string change, but it can land only after a revert (#31098).

An alternative is to update v8_embedder_string separately, PR: #31096.

ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Dec 26, 2019
@ChALkeR
Revert "deps: V8: cherry-pick 687d865fe251"
f058231 
This reverts commit 0e21c1e.

It landed without a proper v8_embedder_string bump, reverting
to re-land cleanly.

Refs: 0e21c1e
Refs: nodejs#31007
@ChALkeR ChALkeR mentioned this pull request Dec 26, 2019
Closed
@BridgeAR
Copy link
Member

@ChALkeR thanks for following up on this! I guess I force pushed to my fork instead of upstream.

BridgeAR pushed a commit that referenced this pull request Jan 1, 2020
@ChALkeR @BridgeAR
deps: V8: bump v8_embedder_string for 0e21c1e
9b0cbcf 
0e21c1e has landed without a proper
v8_embedder_string bump, this is a follow-up fix.

PR-URL: #31096
Refs: 0e21c1e
Refs: #31007
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2020

Closing as resolved.

@BridgeAR BridgeAR closed this Jan 2, 2020
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
@ChALkeR @BridgeAR
deps: V8: cherry-pick 687d865fe251
647f3c7 
Original commit message:

    [heap] Perform GCs on v8::BackingStore allocation

    This adds heuristics to perform young and full GCs on allocation
    of external ArrayBuffer backing stores.

    Young GCs are performed proactively based on the external backing
    store bytes for the young generation. Full GCs are performed only
    if the allocation fails. Subsequent CLs will add heuristics to
    start incremental full GCs based on the external backing store bytes.

    This will allow us to remove AdjustAmountOfExternalMemory for
    ArrayBuffers.

    Bug: v8:9701, chromium:1008938
    Change-Id: I0e8688f582989518926c38260b5cf14e2ca93f84
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1803614
    Commit-Queue: Ulan Degenbaev <[email protected]>
    Reviewed-by: Dominik Inführ <[email protected]>
    Reviewed-by: Hannes Payer <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#65480}

PR-URL: #31007
Refs: v8/v8@687d865
Refs: #1671
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
@ChALkeR @BridgeAR
deps: V8: bump v8_embedder_string for 0e21c1e
e2ef1a9 
0e21c1e has landed without a proper
v8_embedder_string bump, this is a follow-up fix.

PR-URL: #31096
Refs: 0e21c1e
Refs: #31007
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
Merged
@targos
Copy link
Member

targos commented Jan 14, 2020

This needs a backport to land on v12.x-staging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devsnek devsnek devsnek approved these changes

@targos targos targos approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@mscdex mscdex Awaiting requested review from mscdex

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. memory Issues and PRs related to the memory management or memory footprint. performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ChALkeR @nodejs-github-bot @BridgeAR @targos @Trott @addaleax @devsnek